You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Currently, these variables are always overridden in the call the wait_for_display(). Before the upgrade to ffmpeg-7.0.2 (PR #2374), the SE_SCREEN_WIDTH and SE_SCREEN_HEIGHT variables were respected in the case where no video uploading was done because wait_for_display() was never called. After the upgrade to 7.0.2, the function is always called and overrides the VIDEO_SIZE variable.
Now, if both screen width and height are specified, VIDEO_SIZE is not overridden and users can control the recording size again.
Motivation and Context
In versions earlier than selenium/video:ffmpeg-7.0.2, the dimensions for recordings are SE_SCREEN_WIDTH x SE_SCREEN_HEIGHT of the selenium/video container. In later versions, the dimensions are SE_SCREEN_WIDTH x SE_SCREEN_HEIGHT of the Selenium node container. This breaks some workflows because of the unexpected recording size.
To reproduce the issue, this docker-compose.yml file can be started and then stopped:
When using selenium/video:ffmpeg-7.0.2-20240829 or newer images, the recording size is 1920x150 px but with selenium/video:ffmpeg-7.0.1-20240820, the recording is 306x46 px. With these changes the recording is once again 306x46 px.
If either of SE_SCREEN_WIDTH or SE_SCREEN_HEIGHT is missing then the behavior falls back to using the node's resolution.
Types of changes
Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Currently, these variables are always overridden in the call the
wait_for_display(). Before the upgrade to ffmpeg-7.0.2 (PR SeleniumHQ#2374), the
SE_SCREEN_WIDTH and SE_SCREEN_HEIGHT variables were respected in the
case where no video uploading was done because wait_for_display() was
never called. After the upgrade to 7.0.2, it the function is always
called and overrides the VIDEO_SIZE variable.
Now, if both screen width and height are specified, the VIDEO_SIZE is
not overridden and users can control the recording size again.
Here are some key observations to aid the review process:
⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 Security concerns
Command Injection: The script uses unquoted variables in shell commands and awk expressions which could potentially allow command injection if the environment variables contain malicious values. The DISPLAY_CONTAINER_NAME and other variables should be properly quoted when used in commands.
The script should validate that SE_SCREEN_WIDTH and SE_SCREEN_HEIGHT contain valid positive integer values when they are set, to avoid potential ffmpeg recording issues with invalid dimensions
if [ -z "$SE_SCREEN_WIDTH" -o -z "$SE_SCREEN_HEIGHT" ]; then
VIDEO_SIZE=$(xdpyinfo | grep 'dimensions:' | awk '{print $2}')
+else+ VIDEO_SIZE="${SE_SCREEN_WIDTH}x${SE_SCREEN_HEIGHT}"
fi
Apply this suggestion
Suggestion importance[1-10]: 9
Why: This suggestion fixes a critical issue where VIDEO_SIZE might be undefined when SE_SCREEN dimensions are provided, which could cause the script to fail or behave unexpectedly when displaying dimensions.
9
Validate screen dimension inputs
Add validation for SE_SCREEN_WIDTH and SE_SCREEN_HEIGHT values to ensure they are positive integers when provided
if [ -z "$SE_SCREEN_WIDTH" -o -z "$SE_SCREEN_HEIGHT" ]; then
VIDEO_SIZE=$(xdpyinfo | grep 'dimensions:' | awk '{print $2}')
+else+ if [[ "$SE_SCREEN_WIDTH" =~ ^[0-9]+$ ]] && [[ "$SE_SCREEN_HEIGHT" =~ ^[0-9]+$ ]] && [ "$SE_SCREEN_WIDTH" -gt 0 ] && [ "$SE_SCREEN_HEIGHT" -gt 0 ]; then+ VIDEO_SIZE="${SE_SCREEN_WIDTH}x${SE_SCREEN_HEIGHT}"+ else+ echo "Error: SE_SCREEN_WIDTH and SE_SCREEN_HEIGHT must be positive integers"+ exit 1+ fi
fi
Apply this suggestion
Suggestion importance[1-10]: 8
Why: The suggestion adds crucial input validation for screen dimensions, preventing potential errors from invalid values and making the code more robust by ensuring only positive integers are accepted.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
User description
Description
Currently, these variables are always overridden in the call the wait_for_display(). Before the upgrade to ffmpeg-7.0.2 (PR #2374), the SE_SCREEN_WIDTH and SE_SCREEN_HEIGHT variables were respected in the case where no video uploading was done because wait_for_display() was never called. After the upgrade to 7.0.2, the function is always called and overrides the VIDEO_SIZE variable.
Now, if both screen width and height are specified, VIDEO_SIZE is not overridden and users can control the recording size again.
Motivation and Context
In versions earlier than
selenium/video:ffmpeg-7.0.2, the dimensions for recordings are SE_SCREEN_WIDTH x SE_SCREEN_HEIGHT of theselenium/videocontainer. In later versions, the dimensions are SE_SCREEN_WIDTH x SE_SCREEN_HEIGHT of the Selenium node container. This breaks some workflows because of the unexpected recording size.To reproduce the issue, this
docker-compose.ymlfile can be started and then stopped:When using
selenium/video:ffmpeg-7.0.2-20240829or newer images, the recording size is 1920x150 px but withselenium/video:ffmpeg-7.0.1-20240820, the recording is 306x46 px. With these changes the recording is once again 306x46 px.If either of SE_SCREEN_WIDTH or SE_SCREEN_HEIGHT is missing then the behavior falls back to using the node's resolution.
Types of changes
Checklist
PR Type
Bug fix
Description
Ensure
SE_SCREEN_WIDTHandSE_SCREEN_HEIGHTvariables control recording size.Prevent overriding
VIDEO_SIZEwhen screen dimensions are specified.Remove default screen width and height from Dockerfile environment variables.
Changes walkthrough 📝
video.sh
Conditional handling of `VIDEO_SIZE` in `video.sh`Video/video.sh
SE_SCREEN_WIDTHandSE_SCREEN_HEIGHT.VIDEO_SIZEif screen dimensions are set.Dockerfile
Removed default screen dimensions from DockerfileVideo/Dockerfile
SE_SCREEN_WIDTHandSE_SCREEN_HEIGHT.